Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Refactor admin audit app #39192

Closed
wants to merge 9 commits into from
Closed

Refactor admin audit app #39192

wants to merge 9 commits into from

Conversation

danialRahimy
Copy link
Contributor

@danialRahimy danialRahimy commented Jul 6, 2023 •
edited
Loading

Summary

Refactor admin audit app

TODO

Checklist

@solracsf solracsf added 3. to review Waiting for reviews technical debt labels Jul 6, 2023
@solracsf solracsf added this to the Nextcloud 28 milestone Jul 6, 2023
@danialRahimy
Copy link
Contributor Author

Hey @szaimen

I opened this PR last week, but haven't been assigned to any reviewers yet. Is there anything that I could do?

Thanks.

@shdehnavi shdehnavi mentioned this pull request Jul 13, 2023
Merged
@szaimen szaimen requested review from blizzz, a team, ArtificialOwl and icewind1991 and removed request for a team July 13, 2023 20:09
@szaimen
Copy link
Contributor

szaimen commented Jul 13, 2023

Thanks for the ping! I requested some reviewers.

@danialRahimy
Copy link
Contributor Author

Dear @szaimen

Would it be necessary for me to take any action in order to proceed with the pull request?

@szaimen
Copy link
Contributor

szaimen commented Aug 16, 2023

@nextcloud/server-backend please review!

@susnux susnux requested review from Altahrim and nfebe August 16, 2023 23:39
Altahrim
Altahrim reviewed Aug 17, 2023
View reviewed changes
apps/admin_audit/lib/AuditLogger.php Outdated Show resolved Hide resolved
apps/admin_audit/lib/Actions/Sharing.php
IShare::TYPE_REMOTE_GROUP => $this->sharedRemoteGroupType($params),
IShare::TYPE_DECK => $this->sharedDeckType($params),
IShare::TYPE_SCIENCEMESH => $this->sharedSciencemeshType($params),
default => null,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if this one should fail (or at least log) if an unknown type is used.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Before my modifications, if a type didn't match the conditions, no action was taken.
I'm uncertain whether to implement logging or throw a specialized exception to handle unknown types.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here, maybe it will ring a bell for someone :)

fsamapoor
fsamapoor reviewed Aug 23, 2023
View reviewed changes
Copy link
Member

@fsamapoor fsamapoor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In my opinion, removing redundant annotations make the code that much cleaner. When a method's parameter is typed, having the exact type hint as an annotation does not add any value to the mix. I think it would be great if you could also review the annotations in this PR.

I would also recommend writing a summary of the changes you have made in your PR's description and explaining briefly how those changes would improve the codebase.

come-nc
come-nc reviewed Aug 28, 2023
View reviewed changes
Copy link
Contributor

@come-nc come-nc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is only moving things around, and is using match in a weird way.
I’m not sure I get the point.

apps/admin_audit/lib/Actions/Sharing.php
]
);
}
match ($params['shareType']) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The use of match when the return value is not used is really confusing.

apps/admin_audit/lib/AppInfo/Application.php
@@ -60,6 +60,7 @@
use OCP\EventDispatcher\IEventDispatcher;
use OCP\IConfig;
use OCP\IGroupManager;
use OCP\IServerContainer;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why? It does not seem used.

danialRahimy and others added 7 commits October 10, 2023 09:40
@danialRahimy
Revert condition to iterate early in the loop
7974ab4 
Signed-off-by: Danial Rahimi <48244647+danialRahimy@users.noreply.github.com>
@danialRahimy
Extract codes to separate methods to more readability
202906a 
Signed-off-by: Danial Rahimi <48244647+danialRahimy@users.noreply.github.com>
@danialRahimy
Replace conditions with match expression according to php8
e17c223 
Signed-off-by: Danial Rahimi <48244647+danialRahimy@users.noreply.github.com>
@danialRahimy
Declare methods return types
9a51a0d 
Signed-off-by: Danial Rahimi <48244647+danialRahimy@users.noreply.github.com>
@danialRahimy
Add properties type
b06913d 
Signed-off-by: Danial Rahimi <48244647+danialRahimy@users.noreply.github.com>
@danialRahimy
Run php-cs-fixer
47e4add 
Signed-off-by: Danial Rahimi <48244647+danialRahimy@users.noreply.github.com>
@danialRahimy
Remove property type to fix psalm error
009983e 
Signed-off-by: Danial Rahimi <48244647+danialRahimy@users.noreply.github.com>
danialRahimy and others added 2 commits October 10, 2023 09:40
@danialRahimy @Altahrim
Update apps/admin_audit/lib/AuditLogger.php
b01dd7c 
Co-authored-by: Benjamin Gaussorgues <github-fa3ie@altahrim.net>
Signed-off-by: danial rahimy <48244647+danialRahimy@users.noreply.github.com>
@danialRahimy
Updated array usage from array() to [] for improved code readability …
5db849f 
…and modern syntax.

Signed-off-by: Danial Rahimi <48244647+danialRahimy@users.noreply.github.com>
@skjnldsv skjnldsv mentioned this pull request Nov 1, 2023
Merged
This was referenced Nov 6, 2023
Merged
Merged
This was referenced Nov 14, 2023
Merged
Merged
@blizzz blizzz modified the milestones: Nextcloud 28, Nextcloud 29 Nov 23, 2023
This was referenced Mar 12, 2024
Merged
Merged
Merged
This was referenced Mar 20, 2024
Merged
Merged
@skjnldsv skjnldsv mentioned this pull request Mar 28, 2024
Merged
81 tasks
@skjnldsv skjnldsv modified the milestones: Nextcloud 29, Nextcloud 30 Mar 28, 2024
@skjnldsv skjnldsv added 2. developing Work in progress stale Ticket or PR with no recent activity and removed 3. to review Waiting for reviews labels Jul 27, 2024
This was referenced Jul 30, 2024
Merged
Merged
This was referenced Aug 5, 2024
Merged
Merged
@skjnldsv skjnldsv mentioned this pull request Aug 13, 2024
Merged
@skjnldsv skjnldsv closed this Aug 14, 2024
@skjnldsv skjnldsv removed this from the Nextcloud 30 milestone Aug 14, 2024
@skjnldsv
Copy link
Member

Hey @danialRahimy thanks for the PR
Unfortunately there seem to be quite a bit of conflicts and some comments left unanswered above, so we closed that one. If you want to re-open and ork on it again, feel free to do so and ping me, we'll be happy to help you and get it move forward

If you used a tooling to create those, feel free to run it again now and ping me @skjnldsv straight away! I'll help you get it merged faster 🚀

Have a nice day :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@come-nc come-nc come-nc left review comments

@fsamapoor fsamapoor fsamapoor left review comments

@Altahrim Altahrim Altahrim approved these changes

@blizzz blizzz Awaiting requested review from blizzz

@ArtificialOwl ArtificialOwl Awaiting requested review from ArtificialOwl ArtificialOwl was automatically assigned from nextcloud/server-backend

@icewind1991 icewind1991 Awaiting requested review from icewind1991 icewind1991 was automatically assigned from nextcloud/server-backend

@nfebe nfebe Awaiting requested review from nfebe

Assignees
No one assigned
Labels
2. developing Work in progress stale Ticket or PR with no recent activity technical debt
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@danialRahimy @szaimen @skjnldsv @Altahrim @fsamapoor @come-nc @blizzz @solracsf